-
Couldn't load subscription status.
- Fork 552
chore(types): Type-clean server/ (20 errors) #1397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Converting to draft while I rebase on the latest changes to develop. |
85c4a12 to
68510da
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Rebased this PR on the latest develop branch, this is ready for review now @Pouyanpi , @cparisien , @trebedea |
6b82ad0 to
452d4e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR implements systematic type-cleaning of the nemoguardrails/server/ module to achieve Pyright compliance. The changes add explicit type annotations throughout the server code, introduce a custom GuardrailsApp class to replace dynamic attribute assignments on the FastAPI instance, make API model fields optional where appropriate, and enforce consistent response structures. The most significant change normalizes LLM response handling by ensuring bot_message is always a dictionary (wrapping string responses in a standard message structure), which creates consistency for downstream JSON serialization. Additionally, the PR fixes a decorator bug where on_any_event was incorrectly marked as @staticmethod, and it makes the aioredis import optional with a helpful runtime error for environments that don't use Redis. These changes integrate with the existing pre-commit hooks by adding nemoguardrails/server/** to the Pyright configuration in pyproject.toml.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/server/api.py | 3/5 | Type-cleaned with custom GuardrailsApp class, LLM response normalization logic, explicit None checks, and consistent response models |
| pyproject.toml | 5/5 | Added nemoguardrails/server/** to Pyright include paths for automated type-checking |
| nemoguardrails/server/datastore/redis_store.py | 5/5 | Made aioredis import optional with runtime validation and type-ignore directives |
Confidence score: 3/5
- This PR requires careful review due to assumptions about LLM response types and new failure modes
- Score reflects that the LLM response normalization assumes non-string responses are always valid dicts (lines 473-478 in api.py), which could fail silently if the model returns unexpected types; explicit None checks add new ValueError/GuardrailsConfigurationError paths (lines 355, 398) that may surface in edge cases not covered by existing tests; making
messagesfields optional changes the API contract though the fallback to empty list should handle most cases - Pay close attention to nemoguardrails/server/api.py, particularly the
bot_messagenormalization logic and the new error paths for None validation
Sequence Diagram
sequenceDiagram
participant User
participant FastAPI as FastAPI App
participant Endpoint as /v1/chat/completions
participant Validation as Pydantic RequestBody
participant Rails as _get_rails()
participant LLMRails as LLMRails Instance
participant Response as ResponseBody
User->>FastAPI: POST /v1/chat/completions
FastAPI->>Endpoint: Route request
Endpoint->>Validation: Validate RequestBody
alt config_id and config_ids both provided
Validation-->>Endpoint: ValueError: Only one allowed
else no config_id or config_ids
Validation->>Validation: Use default_config_id
alt no default config
Validation-->>Endpoint: GuardrailsConfigurationError
end
end
Validation->>Validation: Ensure config_ids is List[str]
Validation-->>Endpoint: Valid RequestBody
Endpoint->>Rails: _get_rails(config_ids)
alt config_ids is None
Rails-->>Endpoint: GuardrailsConfigurationError
end
Rails->>Rails: Check cache for config_ids
alt not in cache
Rails->>Rails: Load RailsConfig from path
alt full_llm_rails_config is None
Rails-->>Endpoint: ValueError: No valid config
end
Rails->>LLMRails: Initialize with config
Rails->>Rails: Store in cache
end
Rails-->>Endpoint: LLMRails instance
Endpoint->>Endpoint: Prepare messages (messages or [])
alt streaming enabled
Endpoint->>LLMRails: generate_async(streaming=True)
LLMRails-->>User: StreamingResponse
else normal generation
Endpoint->>LLMRails: generate_async()
LLMRails-->>Endpoint: GenerationResponse
Endpoint->>Endpoint: Extract bot_message_content
alt bot_message_content is str
Endpoint->>Endpoint: Wrap in dict with role/content
else already dict
Endpoint->>Endpoint: Use as-is
end
Endpoint->>Response: Create ResponseBody(messages=[bot_message])
Response-->>User: Return JSON response
end
3 files reviewed, 4 comments
| if spec is not None and spec.loader is not None: | ||
| config_module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(config_module) | ||
| else: | ||
| config_module = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Check for None added before using spec.loader to prevent AttributeError if spec_from_file_location fails
| # Ensure config_ids is not None before passing to _get_rails | ||
| if config_ids is None: | ||
| raise GuardrailsConfigurationError("No valid configuration IDs available.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: This check is redundant—lines 389-395 already ensure config_ids is set or an error is raised.
| bot_message_content = res.response[0] | ||
| # Ensure bot_message is always a dict | ||
| if isinstance(bot_message_content, str): | ||
| bot_message = {"role": "assistant", "content": bot_message_content} | ||
| else: | ||
| bot_message = bot_message_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Assumes non-string response is always a valid dict. If the LLM returns an unexpected type (int, list, etc.), it will cause errors downstream. What types can res.response[0] actually be? Should we validate the structure of non-string responses?
| if body.thread_id and datastore is not None and datastore_key is not None: | ||
| await datastore.set(datastore_key, json.dumps(messages + [bot_message])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: datastore_key can only be None if body.thread_id is falsy (line 423), but the check on line 485 already ensures body.thread_id is truthy. The datastore_key is not None part is redundant.
Description
Type-cleaned the nemoguardrails/server directory to get it clean according to Pyright. Added the directory to be automatically checked by pyright in the pre-commits.
Type-cleaning
This report summarizes the type-safety fixes implemented in the pull request. The changes have been categorized by their potential risk of disrupting existing functionality.
🔴 High Risk
This change involves significant assumptions about data structures and alters runtime logic to enforce type consistency.
nemoguardrails/server/api.py, Line 451res.response[0]could be astror adict. Assigning it directly tobot_messagecreated an inconsistent type, which could cause errors in downstream processing or when serializing the final response.bot_messageis always adict, creating a consistent data type for the rest of the function.bot_message_contentis not astr, it must be adictthat already conforms to the required message structure. If the model were to return another data type (e.g., an integer), it would pass through and likely cause an error later.bot_message_contentwith validation, which would explicitly handle malformed responses instead of implicitly trusting the structure. However, the current fix is a pragmatic solution for the common cases.🟠 Medium Risk
These changes modify API contracts, introduce new failure modes, or alter control flow to handle potential
Nonevalues. They are generally safe but represent a stricter enforcement of types.Type: Making API Model Fields Optional
nemoguardrails/server/api.py, Lines 189 & 235messagesfield inRequestBodyandResponseBodywas required (List[dict]), but in practice, it might be omitted. This could lead to validation errors.Optional[List[dict]].messagesfield to beNone. To handle this, thechat_completionfunction was updated to default to an empty list ifbody.messagesisNone(messages = body.messages or []), preventing errors downstream.RequestBodywould be to usedefault_factory=list, which would always ensure an empty list is present if the field is omitted. The chosen approach of usingOptionalis also a standard and valid pattern.Type: Enforcing Consistent Response Model
nemoguardrails/server/api.pychat_completionendpoint returned raw dictionaries (dict), which lacked schema enforcement and could lead to inconsistent responses.ResponseBodymodel.ResponseBody, the API response is now validated against a defined schema, improving reliability and self-documentation.ResponseBodymodel.Type: Adding Explicit
NoneChecks and New Error Pathsnemoguardrails/server/api.py, Lines 333 & 371full_llm_rails_configandconfig_idscould potentially beNoneat runtime, leading toAttributeErrororTypeErrorin subsequent code.None.🟢 Low Risk
These changes are simple type hint additions, corrections of obvious bugs, or improvements to developer experience that have no impact on runtime logic.
Type: Adding Type Hints to Variables and Collections
nemoguardrails/server/api.py,nemoguardrails/server/datastore/redis_store.pyregistered_loggersandllm_rails_instances, were untyped, reducing code clarity and preventing effective static analysis.Type: Correcting
staticmethodUsagenemoguardrails/server/api.py, Line 511on_any_eventwas incorrectly marked as a@staticmethod. The parent classFileSystemEventHandlerexpects an instance method, which receivesselfas the first argument.@staticmethoddecorator was removed.Type: Enabling Type Checking for the
serverModulepyproject.toml, Line 159nemoguardrails/server/directory was not included in thepyrightconfiguration, so type errors in this part of the codebase were not being detected.includelist inpyproject.toml.Test Plan
Type-checking
$ poetry run pre-commit run --all-files check yaml...............................................................Passed fix end of files.........................................................Passed trim trailing whitespace.................................................Passed isort (python)...........................................................Passed black....................................................................Passed Insert license in comments...............................................Passed pyright..................................................................PassedUnit-tests
Local CLI check
Checklist